Classic UI: fix dynamic widget visibility and dynamic update of the page title #3495
Conversation
lolodomo
changed the title
| private void addItemWithName(Set<GenericItem> items, String itemName) { | ||
| if (itemName != null) { | ||
| try { | ||
| Item item = itemRegistry.getItem(itemName); |
There was a problem hiding this comment.
I know you just re-factored this code into another method, but I am asking myself why the getItem() method is called on the itemRegistry vs itemUIregistry in case of the basicui.
There was a problem hiding this comment.
You're tight. I changed to itemUIregistry in Classic UI, it still works for my use cases to test the issues I am fixing with this PR. I have not done a full analysis of all possible consequences of this change from itemRegistry to itemUIregistry but I imagine it should be ok as it is with Basic UI.
|
Comment taken into account + changes squashed. |
| @@ -275,7 +289,9 @@ public boolean hasChangeOccurred() { | |||
| */ | |||
| @Override | |||
| public void stateUpdated(Item item, State state) { | |||
| // ignore if the state did not change | |||
| if (item instanceof GroupItem) { | |||
| changed = true; | |||
There was a problem hiding this comment.
This looks pretty dangerous to me.
Note that the Classic UI intentionally had some constraints regarding dynamic updates as every update results in a full page refresh, which lets the screen flicker, scroll and makes the UI difficult to use, if updates are too frequent.
Especially GroupItems receive MANY state updates and thus they are prone to such negative effects.
@lolodomo & @triller-telekom Did you extensively test this on large sitemaps with lots of events? We really should make sure not to cause any major new issues here. Classic UI should really be in maintenance mode and only severe stuff should be fixed on it.
There was a problem hiding this comment.
I did not check this on a very large sitemap and I was not aware of these constraints. But they surely make sense because scrolling and screen flickering isn't nice.
@lolodomo: What did you experiments say when you tested it?
There was a problem hiding this comment.
We are in Home Automation systems, I doubt users have setup with lots of event updates per second. That is not my case.
I think I recently read that for standard groups there is no more any state update. Can you please confirm that point ? If that is true, the change will only concern groups with a function. If that is not true, I should change the code to trigger a reload on state update only for groups having a function.
There was a problem hiding this comment.
Standard groups can still have a state. However, if they do not have a function their state is not derived from their member items.
If you change the code to only refresh the site for groups with a function you will miss state changes on those without function.
|
If you think I introduced a problem, I put in comments my fix relative to the handling of group state and then I switch to something else. At least this PR will fix the handling of visibility attribute and the update of page title for non group items. |
Yes, you might be right - so let's hope that this indeed does not have any "flickering" impact. |
Fix #3488
Fix #3176
Signed-off-by: Laurent Garnier lg.hc@free.fr